-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Construct multi-qubit Pauli terms and sums from strings #984
Conversation
Added support for multi-qubit strings to PauliTerm.from_compact_str Added Tests for all of the above changed: pyquil/paulis.py changed: pyquil/tests/test_paulis.py changed: pyquil/tests/test_paulis_with_placeholders.py
changed: pyquil/tests/test_paulis.py changed: pyquil/tests/test_paulis_with_placeholders.py
and the tests. Made the syntactical changes requested by @msohaibalam Zum Commit vorgemerkte Änderungen: geändert: pyquil/paulis.py geändert: pyquil/tests/test_paulis.py geändert: pyquil/tests/test_paulis_with_placeholders.py
Fixed the dimensions of the resulting matrices, s.t. it produces the same results as pyquil.unitary_tools.lifted_pauli
- Added PauliSum.from_compact_str() - Added support for multiqubit strings to PauliTerm.from_compact_str() - Added PauliSum.compact_str() changed: paulis.py changed: tests/test_paulis.py changed: tests/test_paulis_with_placeholders.py
changed: CHANGELOG.md changed: docs/source/apidocs/pauli.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small docstring typo. Otherwise looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have an opinion on the __init__
question, but otherwise lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlbosse Thanks for the PR! I think the scope of this looks great. However, parsing magic strings can get a little hairy. For example, the following does not work:
In [21]: print(PauliSum.from_compact_str('X0 + X1'))
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-21-b4e8046c399b> in <module>
----> 1 print(PauliSum.from_compact_str('X0 + X1'))
~/code/rigetti/github/pyquil/pyquil/paulis.py in from_compact_str(cls, str_pauli_sum)
704 str_terms = re.split(r'\+(?![^(]*\))', str_pauli_sum)
705 str_terms = [s.strip() for s in str_terms]
--> 706 terms = [PauliTerm.from_compact_str(term) for term in str_terms]
707 return cls(terms)
708
~/code/rigetti/github/pyquil/pyquil/paulis.py in <listcomp>(.0)
704 str_terms = re.split(r'\+(?![^(]*\))', str_pauli_sum)
705 str_terms = [s.strip() for s in str_terms]
--> 706 terms = [PauliTerm.from_compact_str(term) for term in str_terms]
707 return cls(terms)
708
~/code/rigetti/github/pyquil/pyquil/paulis.py in from_compact_str(cls, str_pauli_term)
359 """
360 # split into coefficient and operator
--> 361 str_coef, str_op = str_pauli_term.split('*', maxsplit=1)
362
363 # parse the coefficient
ValueError: not enough values to unpack (expected 2, got 1)
Granted, this example is due to the fact that print(PauliTerm.from_compact_str('X0'))
doesn't work, but there are other "reasonable-looking" strings that you can put in that spit out a confusing error. Thus, I think it would be best to expand upon your unit test, and include every flavor of compact string you can think of. For those that should actually result in an error, we should catch that and return an error message that gives the user a sense of what to do next. Let me know if that makes sense -- happy to add more detail.
Some additional examples:
- this should probably error but doesn't
In [24]: print(PauliTerm.from_compact_str('1+1j*X0'))
(1+1j)*X0
- this should probably collect terms
In [29]: print(PauliSum.from_compact_str("(1.0+0j)*X0 + (1.0+0j)*Y0 + 1*X0"))
(1+0j)*X0 + (1+0j)*Y0 + (1+0j)*X0
- this should probably tell you "A" is the problem
In [30]: PauliSum.from_compact_str("(1.0+0j)*X0 + (1.0+0j)*A0")
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-30-e717615331a7> in <module>
----> 1 PauliSum.from_compact_str("(1.0+0j)*X0 + (1.0+0j)*A0")
~/code/rigetti/github/pyquil/pyquil/paulis.py in from_compact_str(cls, str_pauli_sum)
704 str_terms = re.split(r'\+(?![^(]*\))', str_pauli_sum)
705 str_terms = [s.strip() for s in str_terms]
--> 706 terms = [PauliTerm.from_compact_str(term) for term in str_terms]
707 return cls(terms)
708
~/code/rigetti/github/pyquil/pyquil/paulis.py in <listcomp>(.0)
704 str_terms = re.split(r'\+(?![^(]*\))', str_pauli_sum)
705 str_terms = [s.strip() for s in str_terms]
--> 706 terms = [PauliTerm.from_compact_str(term) for term in str_terms]
707 return cls(terms)
708
~/code/rigetti/github/pyquil/pyquil/paulis.py in from_compact_str(cls, str_pauli_term)
378 str_op = re.sub(r'\*', '', str_op)
379 if not re.match(r'^(([XYZ])(\d+))+$', str_op):
--> 380 raise ValueError(f"Could not parse pauli string {str_pauli_term}")
381
382 for factor in re.finditer(r'([XYZ])(\d+)', str_op):
ValueError: Could not parse pauli string (1.0+0j)*A0
Many (if not all) of these are errors that existed before you opened your PR, so if you'd rather just catch them in your tests as expected errors, open an issue, and leave them for a later PR (as to not blow up the scope of your work) that is totally fine with me. In fact, that might be the best course of action. Let me know what you think! 😃
As for |
Committed changes: changed: paulis.py changed: tests/test_paulis.py
@karalekas Thanks for pointing out the manifold ways the user could specify unparsable strings. I added checks for each part of the string now that point at the exact part where things became unparsable.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding all those tests! I think this is nearly ready, but I think that the test_pauli_sum_from_str
could be expanded as well. Could you test a couple more pauli sum strings that should parse correctly, as well as test the simplification component?
- Test that simplification works correctly - Test that PauliSums of length 1 work correctly - Test that strings containing the identity work correctly Changed files: changed: tests/test_paulis.py
Done! I added tests for the simplification part, strings containing the identity and sums of length one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small fixes to the changelog entry -- also please move it up to the "Improvements and Changes" section for v2.12, instead of v2.11. Probably want to rebase off of master too.
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
Co-Authored-By: Peter Karalekas <peter@rigetti.com>
Incorporated the Changelog changes (and clearly didn't write that fact into the changelog, otherwise I would still be stuck in a recursive loop without exit condition). Rebasing off master has been done commit e761981. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jlbosse Congrats on your first PR contribution to pyQuil! :) |
Description
Added
PauliSum.from_compact_str()
andPauliSum.compact_str()
in analogy toPauliTerm.from_compact_str()
andPaulTerm.compact_str()
.Added support for multi-qubit operators to
PauliTerm.from_compact_str()
.Added corresponding tests.
This resolves #977 mostly (except for the question, whether init() should also be changed).
Checklist
test_examples.test_quantum_die fails, but since I ddin't touch anything remotely related I
am sure I can shift the blame
None of the surrounding code has type hints, so I refrained from adding them.
auto-close keywords.
including author and PR number (@username, gh-xxx).